Skip to content

Use Jason.encode! when sending errors#161

Open
khaapamyaki wants to merge 3 commits into
keathley:masterfrom
khaapamyaki:fix/error-json-encoder
Open

Use Jason.encode! when sending errors#161
khaapamyaki wants to merge 3 commits into
keathley:masterfrom
khaapamyaki:fix/error-json-encoder

Conversation

@khaapamyaki
Copy link
Copy Markdown

@khaapamyaki khaapamyaki commented Apr 15, 2022

At the moment Twirp.Plug.send_error/2 uses Twirp.Encoder.encode/3 for encoding the error.

This leads to a problem where the defimpl Jason.Encoder for the Twirp.Error struct is not being used, and thus the :__exception__ field is being sent with every error response.

The twirp specification v7 describes how error responses must be encoded:

 {
  "code": "permission_denied",
  "msg": "Thou shall not pass",
  "meta": {
    "target": "Balrog",
    "power": "999"
  }
}

code and msg are required while meta is optional.

The twirp-elixir implementation will send the following:

{
  "__exception__": true,
  "code": "permission_denied",
  "msg": "Thou shall not pass",
  "meta": {
    "target": "Balrog",
    "power": "999"
  }
}

The twirp go implementation is very strict about the errors

var tj twerrJSON
dec := json.NewDecoder(bytes.NewReader(respBodyBytes))
dec.DisallowUnknownFields()
if err := dec.Decode(&tj); err != nil || tj.Code == "" {
	// Invalid JSON response; it must be an error from an intermediary.
	msg := fmt.Sprintf("Error from intermediary with HTTP status code %d %q", statusCode, statusText)
	return twirpErrorFromIntermediary(statusCode, msg, string(respBodyBytes))
}

This leads the Go implementation to handle the error as if it originated from an intermediary (e.g. a reverse proxy) and translating the HTTP status code 404 as a bad_route instead of the original error.

This PR makes use of Jason.encode!/1 in the Twirp.Plug.send_error/2 to fix this issue, as it will use the protocol implementation found in Twirp.Error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant